move DD log correlation patch for Active Job to wrap full execution #4916
Conversation
|
Hey @agrobbin ! Thanks for your contribution ! Just a heads up to tell you that I will look at this pull request next week as soon as possible ! |
|
Hey @agrobbin ! Unfortunately this change does not work with Rails < 6.0 (which we still support). It seems that |
agrobbin
force-pushed
the
active-job-log-injection
branch
from
7964aff to
7acecb3
Compare
|
@vpellan interesting! I looked at the source and it seems the logging of Active Job itself was reimplemented in Rails 6+, so it makes sense that this wouldn't work quite the same in those versions. I've introduced branching to only wrap the full execution in Rails 6+, which should ensure that test passes again! |
|
Hey @agrobbin ! Thanks for the fix ! I left a small comment but except that, LGTM ! We need to run these changes on our latest CI before merging, would you agree to give us the authorization to merge master on your branch ? So we don't have to bother you each time we need to run the CI. Thanks ! |
agrobbin
force-pushed
the
active-job-log-injection
branch
from
7acecb3 to
cfe8ae0
Compare
|
Updated! I also rebased off of the latest |
The current Active Job integration injects a log correlation tag into the logger via `around_perform`, which is too late to pick up the following logs: ``` [ActiveJob] [FooJob] [540c4718-6668-4f3c-9411-ca52a4cffd15] Performing FooJob (Job ID: 540c4718-6668-4f3c-9411-ca52a4cffd15) from Sidekiq(default) enqueued at 2025-09-26T02:17:44.160363327Z with arguments: #<GlobalID:0x00007f0fae88cb50 @uri=#<URI::GID gid://foo-rails/Foo/019983cf-8422-72d5-85b7-f3dc09a4887e>> [ActiveJob] [FooJob] [540c4718-6668-4f3c-9411-ca52a4cffd15] Performed FooJob (Job ID: 540c4718-6668-4f3c-9411-ca52a4cffd15) from Sidekiq(default) in 990.29ms ``` Instead of using `around_perform`, we now do the same thing that [Active Job itself](https://github.com/rails/rails/blob/v8.0.3/activejob/lib/active_job/logging.rb#L31-L33) does to tag logs with `[ActiveJob]`, the job class name, and job ID, which is wrapping `perform_now`. Note that this only works on Active Job 6+, so we inject different log correlation logic depending on the Active Job version.
agrobbin
force-pushed
the
active-job-log-injection
branch
from
cfe8ae0 to
6a1f75f
Compare
|
Thank you very much for your contribution, this will be available in the next release ! |
3 participants
What does this PR do?
Instead of using
around_perform, we now do the same thing that Active Job itself does to tag logs with[ActiveJob], the job class name, and job ID, which is wrappingperform_now.Motivation:
The current Active Job integration injects a log correlation tag into the logger via
around_perform, which is too late to pick up the following logs:Change log entry
Additional Notes:
How to test the change?
I looked for an existing test to exercise this, but was unable to find one, and didn't see any tests specifically related to Active Job itself.